Skip to content

doc_suspicious_footnotes: lint text that looks like a footnote #14708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

doc_suspicious_footnotes: lint text that looks like a footnote #14708

wants to merge 3 commits into from

Conversation

notriddle
Copy link
Contributor

changelog: [doc_suspicious_footnotes]: lint for text that looks like a footnote reference but has no definition

This is an alternative to rust-lang/rust#137803, meant to address the concerns about false positives.

This lint only fires when the apparent footnote reference has a name that's made from pure ASCII digits. This choice is justified by running lintcheck on the top 200 crates, plus the clippy default set:

  1. I ran lintcheck with a modded version of this lint that didn't check for digits only. It produced a false positive warning on a line in mdbook that had a regex, and no true positives at all.
  2. I also ran lintcheck with a custom lint that fired on any valid footnote reference with a non-ascii-digit name. cargo uses one in its job_queue module, and that's all it found.

cc @GuillaumeGomez

@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2025

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 29, 2025
@notriddle notriddle force-pushed the doc_suspicious_footnotes branch from 9f74df2 to c56ef28 Compare April 29, 2025 23:48
@notriddle notriddle force-pushed the doc_suspicious_footnotes branch from c56ef28 to 1356e76 Compare April 29, 2025 23:50
@GuillaumeGomez
Copy link
Member

Looks good to me, thanks!

r? samueltardieu

@rustbot
Copy link
Collaborator

rustbot commented May 2, 2025

samueltardieu is on vacation.

Please choose another assignee.

blyxyas
blyxyas requested changes May 4, 2025
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great initial patch (and a really good lint idea 👍)! I left some suggestions and some questions.

tests/ui/doc_suspicious_footnotes.rs
///
/// This is a footnote[^2].
///
/// [^2]: hello world
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this also check for #![doc], //!, #[doc] and the include!s macros?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 4, 2025
@notriddle
Copy link
Contributor Author

@blyxyas Okay, I've implemented variants on all your suggestions.

@notriddle notriddle force-pushed the doc_suspicious_footnotes branch from b0e3325 to 99b16ae Compare May 5, 2025 22:45
blyxyas
blyxyas reviewed May 14, 2025
View reviewed changes
clippy_lints/src/doc/mod.rs Outdated
@@ -1147,7 +1186,8 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize
// Don't check the text associated with external URLs
continue;
}
text_to_check.push((text, range, code_level));
text_to_check.push((text, range.clone(), code_level));
doc_suspicious_footnotes::check(cx, doc, range, &fragments);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure that you've already tried this, but is there a reason that you didn't use FootnoteReference from ķEvent? You would not have to do span magic and manually parse footnotes and it seems that this function doesn't actually use text

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FootnoteReference event is only emitted if the code is actually a footnote. This lint fires for text that looks like a footnote reference but isn't.

@notriddle notriddle force-pushed the doc_suspicious_footnotes branch from 00f6841 to 40040e1 Compare May 19, 2025 23:53
@notriddle
Copy link
Contributor Author

@blyxyas

I've redone the suggestions using attr metadata instead of string manipulation.

This also has test cases for cfg_attr and block doc comments.

@notriddle notriddle force-pushed the doc_suspicious_footnotes branch from 40040e1 to 6c70997 Compare May 20, 2025 00:07
@notriddle notriddle force-pushed the doc_suspicious_footnotes branch from 6c70997 to 98426f5 Compare May 20, 2025 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blyxyas blyxyas blyxyas requested changes

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@blyxyas blyxyas

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@notriddle @rustbot @GuillaumeGomez @blyxyas